Add events task by cecton · Pull Request #76 · aio-libs/aiodocker

cecton · 2017-07-25T10:03:42Z

What These Changes Do

~~Explicit loop~~
Fix a problem with ClientResponse is not a coroutine
DockerEvents automatically create and cancel a background task by default (breaking change)

asvetlov · 2017-07-25T11:19:39Z

aiodocker/docker.py

        self.images = DockerImages(self)
        self.volumes = DockerVolumes(self)

-    async def close(self):


Please revert it back.
session.close() is a coroutine.

@asvetlov sorry it's a work in progress, don't waste your time looking at it at the moment. Thanks for the remark though I was totally mistaking.

codecov · 2017-07-25T14:00:41Z

Codecov Report

Merging #76 into master will increase coverage by 0.63%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #76      +/-   ##
==========================================
+ Coverage   76.92%   77.56%   +0.63%     
==========================================
  Files          12       12              
  Lines         715      722       +7     
==========================================
+ Hits          550      560      +10     
+ Misses        165      162       -3

Impacted Files	Coverage Δ
aiodocker/jsonstream.py	`94.28% <100%> (+2.85%)`	⬆️
aiodocker/docker.py	`69.46% <100%> (+1.18%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5defebb...18ba173. Read the comment docs.

barrachri · 2017-07-25T14:33:05Z

@cecton just ping when the PR is ready for the review.

cecton · 2017-07-26T09:29:53Z

@barrachri ping

So here is the idea I had in #34 : 20965e0

But to do that I had to add the event loop to the Docker instance: 7097cb1

And I also installed tox, removed the need of Docker Compose and made all the test in test_images.py unrelated to each other (so you can run them seperately): bb24437

I also made a lot of changes in the requirements files:

I added requirements.txt: a single place where you put only the library's requirements
I added requirements/test.txt: a single place where you put only the requirements for running the tests
I replaced requirements/ci.txt: it is now a single place without versions where you put the requirements for the CI (codecov)

But I have no idea why setuptools is in the requirements to be honest. I saw that the bot is updating its version but setuptools is only used in the CI itself. It doesn't make much sense to me, I would remove it if I were you.

Also I'm a bit concerned about how the event monitoring is closed. Is the user supposed to call stop() before closing the Docker instance? Why it's not in the close() of the Docker class directly? In the test in integration you just don't call it and it seems work but when I removed the task handling I noticed that the task is destroyed. So I suppose the json_stream was never closed properly explicitly.

cecton · 2017-07-26T10:00:23Z

Hmmm.... I just spotted something fishy with a try...except: pass... there will be more fixes in this PR.

Done.

asvetlov · 2017-07-27T12:10:26Z

aiodocker/docker.py

                 ssl_context=None,
-                 api_version='v1.26'):
+                 api_version='v1.26',
+                 loop=None):


Using implicit loop is preferable technique starting from python 3.5.3
aiodocker is a new library, we don't need to pass loop everywhere.

I didn't know that... look what I found on the web when trying to find more information: https://groups.google.com/forum/#!topic/python-tulip/yF9C-rFpiKk

See also python/asyncio#452

asvetlov · 2017-07-27T12:12:38Z

requirements.txt

@@ -0,0 +1,2 @@
+aiohttp==2.2.3


I don't like this file.
If you need new requirement -- please put it into requirements folder.

requirements.txt is more generally used in Python projects when you look for the requirements. I expect most Python developers to expect either a requirements.txt or no requirements.txt at all because the dependencies are in the setup.py.

My true goal here is only to get one place to maintain the dependencies. I can move it to requirements/base.txt or something if you prefer?

Our requirements are for aiodocker developers, users install the library by pip install aiodocker without explicitly looking into requirement files at all.
Thus doesn't matter where we do place the file.
If we have everything in requirements folder -- let's put base requirements there.
requirements/base.txt sounds pretty good.

asvetlov · 2017-07-27T12:15:24Z

setup.py

-    extras_require={
-        'test': ['pytest', 'pytest-asyncio', 'flake8'],
-    }
+    install_requires=requirements,


I suspect the library should not pin own requirements -- otherwise we need publish new aiodocker on every yarl/aiohttp update.

Nevertheless the lib work with some versions but not all. For aiohttp I suppose we are following semantic versioning so maybe we could use <2.3. What do you think would be best? Simply removing any version restriction?

Relax it to non-strict check, e.g. aiohttp>=2.0 where 2.0 is minimal supported version.
Sorry, I don't follow what minimal aiohttp version is supported by aiodocker.

I will figure that out easily with tox

2.0.0 works, 1.3.5 failed

asvetlov · 2017-07-27T12:16:56Z

Yes, setuptools is not needed in requirements

asvetlov · 2017-07-27T12:18:38Z

Other thing: I think better to split new feature (a task for events monitoring) and improvements of existing infrastructure.

barrachri · 2017-07-26T19:53:21Z

aiodocker/jsonstream.py


        # response error , it has been closed
-        await self._response.close()
+        self._response.close()


Did you forget await?

No I actually fixed it. This is no coroutine apparently and the try...except...pass on the top level was hiding the exception. Commit message: 1d3abd8

barrachri · 2017-07-26T21:31:31Z

tests/test_docker.py

+import asyncio
+
+import pytest
+


import asyncio import aiohttp import aiodocker import pytest

barrachri · 2017-07-26T21:36:59Z

tests/test_images.py

 from aiodocker.exceptions import DockerError


+def _random_name():


Great idea 👍

barrachri · 2017-07-26T21:38:49Z

tests/test_images.py

@@ -1,21 +1,25 @@
 import pytest
+import uuid


import uuid from io import BytesIO import pytest from aiodocker import utils from aiodocker.exceptions import DockerError

barrachri · 2017-07-26T21:40:03Z

tests/test_images.py

-    async for output in stream:
-        if "Successfully tagged image:1.0\n" in output:
-            break
+    async for _ in stream:  # noqa: F841


why looping if you don't check also the output of the stream?

That's the funny part of this PR. If you don't loop, you're image will not finish to build and the code will break. Maybe that only happens if the stream=True is active though. I know it's the case for docker-py and I just checked with a big image with aiodocker and it behaves the same.

Now the funny part is that the previous code has problems too. I first tried to use it and catch from the output and assert on the output. But I realized that the "output" is no string, it's a dict, and my assert failed. The actual log line is in output['stream'].

So I updated the code and then the test failed because depending on the version of Docker, you may get an upper case, a lower case here and there, etc...

So in the end I felt like the only interesting thing to do here is to wait for the build to complete because this for loop doesn't actually do anything else than that.

So actually we can remove the for loop with the stream=True and it will behave the same.

barrachri · 2017-07-26T21:43:43Z

tests/test_images.py

              "aiodocker/master/tests/docker/Dockerfile")

-    tag = "image:1.0"
+    tag = "%s:1.0" % _random_name()


Can you switch to "".format()?

I just like to be consistent around the library.

I checked that in the past and apparently the devs at Python don't want to drop the % syntax because is handy and small to use. Plus they even added a new one:

you = "barrachri" print(f"Hello {you}")

So.... well I disagree but I really don't mind at all changing it for consistency sake.

As I said it just a matter of consistency :)
Remember that is .format() and not f"", that is available only from python 3.6.

Yes but % is not going to be deprecated and is available ^_^ but anyway...

cecton · 2017-07-29T08:41:28Z

@asvetlov @barrachri okay, ready for review.

cecton · 2017-07-29T08:43:12Z

aiodocker/docker.py

-            try:
-                await self.json_stream.close()
-            except:
-                pass


@barrachri the error in the close() used to be hidden by this try...except: pass

Ok, it seems harmless, I'll check this .close() in the future PRs.

barrachri · 2017-08-01T15:13:21Z

@cecton I plan to close the review tomorrow, I didn't find time during the weekend :/

cecton · 2017-08-01T15:42:11Z

@barrachri no problem for me! :) Thanks.

To be honest, at this point I'm more evaluating aiodocker to see if I can replace my own one. I'm still very dubious about it especially since I discovered how I can properly override the methods in docker-py.

asvetlov · 2017-08-01T17:29:15Z

@barrachri please feel free to merge if the PR looks good to you.

BTW we can raise election of @cecton to aiodocker team.
She is a member of aiohttp already, why not?

barrachri · 2017-08-01T17:41:17Z

@cecton I re-run the tests after the last merges, there seems to be some errors.

Can you have a look?

cecton · 2017-08-01T18:09:27Z

@barrachri okay I just checked and close() on _JsonStreamResult has been renamed to _close. Should I simply use that one?

barrachri · 2017-08-01T18:10:40Z

Yes, thanks!

cecton · 2017-08-01T18:33:59Z

@barrachri done

barrachri · 2017-08-01T18:50:32Z

Thanks @cecton !

asvetlov requested changes Jul 25, 2017

View reviewed changes

cecton force-pushed the add-events-task branch 3 times, most recently from d82456b to d278c31 Compare July 25, 2017 13:52

cecton force-pushed the add-events-task branch 3 times, most recently from 5e31d94 to 20965e0 Compare July 26, 2017 09:24

cecton force-pushed the add-events-task branch from e2de8cf to 96810f8 Compare July 26, 2017 10:03

cecton changed the title ~~WIP Add events task~~ Add events task Jul 27, 2017

cecton force-pushed the add-events-task branch from 96810f8 to 1d3abd8 Compare July 27, 2017 08:31

asvetlov requested changes Jul 27, 2017

View reviewed changes

barrachri reviewed Jul 27, 2017

View reviewed changes

cecton force-pushed the add-events-task branch 3 times, most recently from f1778d7 to f151ffa Compare July 29, 2017 08:25

cecton commented Jul 29, 2017

View reviewed changes

asvetlov approved these changes Aug 1, 2017

View reviewed changes

cecton added 2 commits August 1, 2017 20:22

Automatically create task for events

4799e00

Fix: ClientResponse.close() is not a coroutine

18ba173

cecton force-pushed the add-events-task branch from f151ffa to 18ba173 Compare August 1, 2017 18:24

barrachri merged commit db74893 into aio-libs:master Aug 1, 2017

cecton deleted the add-events-task branch August 1, 2017 20:27

		from aiodocker.exceptions import DockerError


		def _random_name():

Uh oh!

Conversation

cecton commented Jul 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What These Changes Do

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jul 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

barrachri commented Jul 25, 2017

Uh oh!

cecton commented Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cecton commented Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cecton Jul 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asvetlov commented Jul 27, 2017

Uh oh!

asvetlov commented Jul 27, 2017

Uh oh!

barrachri Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

barrachri Jul 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cecton commented Jul 25, 2017 •

edited

Loading

codecov bot commented Jul 25, 2017 •

edited

Loading

cecton commented Jul 26, 2017 •

edited

Loading

cecton commented Jul 26, 2017 •

edited

Loading

cecton Jul 27, 2017 •

edited

Loading

barrachri Jul 26, 2017 •

edited

Loading

barrachri Jul 27, 2017 •

edited

Loading